refactor/nullable repo key by fiftin · Pull Request #3826 · semaphoreui/semaphore

fiftin · 2026-05-03T15:33:15Z

refactor(repos): allow nil for key id
feat(migration): nullable key column in repo

cursor

Stale comment

Security review (PR 3826)

Outcome: One medium finding: nullable ssh_key_id without defensive checks on Repository.SSHKey allows authenticated denial-of-service (process panic) on git operations and unsafe behavior where code assumes a non-nil key.

Details: ValidateRepository skips key validation when SSHKeyID is nil, but several paths still dereference SSHKey (notably GoGitClient.getAuthMethod and Repository.GetGitURL for HTTP). Any project user who can create/update repositories can omit ssh_key_id and trigger panics or inconsistent auth when the scheduler/runner hits git.

No separate high/critical authz bypass or injection issue was confirmed in the reviewed diff; runner/task secret mapping changes correctly gate on SSHKeyID != nil.

Slack summary: PR 3826 security review: 1 medium — nullable repo SSH key + missing nil guards → DoS (panic) on git paths; fix by guarding SSHKey == nil before use or rejecting null key for remote repos in validation.

_{Sent by Cursor Automation: Find vulnerabilities}

cursor · 2026-05-03T15:35:36Z

@@ -42,12 +42,14 @@ func (c GoGitClient) getAuthMethod(r GitRepository) (transport.AuthMethod, error
 	switch r.Repository.SSHKey.Type {


Severity: Medium — denial of service (availability)

ValidateRepository returns without loading a key when SSHKeyID is nil, so Repository.SSHKey can be nil at runtime for normal API-created repos. This function still does switch r.Repository.SSHKey.Type, which panics on nil before any auth branch. Any authenticated user who can add a remote repository without ssh_key_id can cause the server process to crash when schedules, tasks, or branch listing run git via the default go-git client.

Impact: Process panic → denial of service for the Semaphore instance.

cursor · 2026-05-03T15:35:36Z

+	SSHKey *AccessKey `db:"-" json:"-" backup:"-"`
 }

 func (r Repository) ClearCache() error {


Severity: Medium — denial of service / unsafe git URL handling

For RepositoryHTTP, GetGitURL(false) uses r.SSHKey.Type and r.SSHKey.LoginPassword without checking SSHKey != nil. With nullable ssh_key_id, SSHKey can be nil when this runs (e.g. cmd-git GetGitURL(false)), causing a nil pointer panic or undefined behavior.

Impact: Same DoS class as GoGitClient.getAuthMethod; also risks incorrect clone URLs if execution continued without crashing.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2a0dc9d091

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-05-03T15:37:50Z

@@ -42,12 +42,14 @@ func (c GoGitClient) getAuthMethod(r GitRepository) (transport.AuthMethod, error
 	switch r.Repository.SSHKey.Type {


Guard nil SSH key before selecting auth method

Repository.SSHKey is now nullable, but getAuthMethod immediately dereferences it in switch r.Repository.SSHKey.Type. For repositories created without a key (e.g., public HTTPS repos), any go-git operation that calls this path will panic before reaching the later nil check, so clone/pull/fetch can crash instead of using unauthenticated access.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-05-03T15:37:50Z

@@ -0,0 +1 @@
+-- do nothing


Rename 2.18.7 rollback file to expected pattern

The rollback loader resolves files as v<version>.err.sql (via getVersionErrPath), so v.2.18.7.err.sql is never discovered. That means rollback handling for migration 2.18.7 silently skips its SQL, which can leave schema changes in place while rollback flow proceeds as if it succeeded.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-05-03T15:37:50Z

+	if d.GetDialect() == util.DbDriverSQLite {
+		_, err = d.Sql().Exec("PRAGMA foreign_keys = OFF")
+		if err != nil {


Restore SQLite foreign_keys when migration exits early

SQLite foreign keys are turned off before starting the transaction, but turned back on only after a successful commit. Several earlier return paths in this function (e.g., begin failure, pre/post-apply errors, insert failure) bypass the re-enable step, so a migration error can leave the process with foreign key enforcement disabled.

Useful? React with 👍 / 👎.

cursor

Stale comment

Security review (PR #3826)

Verdict: One high-confidence availability issue was introduced in db_lib/GoGitClient.go when Repository.SSHKey became nullable: getAuthMethod still switches on r.Repository.SSHKey.Type before verifying the pointer, so a repository with no SSH key (nullable ssh_key_id) and a non-local URL causes a nil pointer dereference (panic) when the built-in go_git client runs (clone/pull/etc.). That is a denial-of-service risk for the runner/task process, not a confidentiality bypass.

Not treated as security findings: SQLite PRAGMA foreign_keys toggles during migration are migration mechanics; runner/API changes around optional keys align with the schema change and do not by themselves expose new authz bypasses. CmdGitClient safely no-ops Destroy() on a zero AccessKeyInstallation.

No prior thread IDs were available in-repo; cleanup_previous was used to reconcile earlier automation assessments.

_{Sent by Cursor Automation: Find vulnerabilities}

cursor · 2026-05-03T16:25:52Z

@@ -42,12 +42,14 @@ func (c GoGitClient) getAuthMethod(r GitRepository) (transport.AuthMethod, error
 	switch r.Repository.SSHKey.Type {


Severity: High (availability / DoS)

Repository.SSHKey is now optional, but this line still dereferences it unconditionally. For a repo with SSHKeyID == nil and a normal https:// URL, GetType() is not RepositoryLocal, so git operations call getAuthMethod, hit switch r.Repository.SSHKey.Type, and panic (nil pointer). Any project user who can create such a repo can break task execution for runners using go_git.

Fix direction: Guard the whole switch with if r.Repository.SSHKey == nil { return nil, nil } (or equivalent) before reading .Type, and treat nil key like AccessKeyNone for HTTPS.

cursor

Stale comment

Security review (PR #3826)

Outcome: One medium finding remains: a nil-pointer path in GoGitClient.getAuthMethod when a repository has no SSH key (nullable ssh_key_id). Other changes (runner payload filtering, migration PRAGMA foreign_keys, backup/restore) do not introduce a plausible auth bypass or injection in reviewed paths.

Notable non-security note: Repository.SSHKeyID still carries binding:"required" while the DB column is now nullable—API validation may be inconsistent with the schema (behavioral, not classified as a vuln here).

_{Sent by Cursor Automation: Find vulnerabilities}

cursor · 2026-05-04T19:20:03Z

@@ -42,12 +42,14 @@ func (c GoGitClient) getAuthMethod(r GitRepository) (transport.AuthMethod, error
 	switch r.Repository.SSHKey.Type {


Medium — denial of service (nil dereference)

Repositories can now have ssh_key_id / SSHKey unset. This function still does switch r.Repository.SSHKey.Type and later dereferences r.Repository.SSHKey (e.g. SSH branch). If SSHKey is nil, the server panics on clone/pull/list branches for that repo.

Impact: Any user who can create/update a repo to omit the key (or any code path that loads such a row) can trigger repeated crashes against endpoints using the Go Git client (e.g. branch listing), affecting availability.

Fix direction: Guard at the top of getAuthMethod (nil SSHKey → return nil, nil or explicit error) before the switch, and ensure SSH/HTTP branches never dereference nil.

cursor

Security review (PR 3826)

Outcome: One medium finding related to the new SQLite migration path.

The nullable ssh_key_id / pointer refactors and runner payload changes were reviewed for authz bypasses, nil dereferences, and secret exposure. Those paths look like intentional hardening (guarding deserialization and map lookups when no key is bound). No high/critical issues identified there.

Finding: ApplyMigration now turns SQLite foreign_keys off before the migration transaction, but several error returns between that point and the successful Commit exit without turning enforcement back on. Any caller that reuses the same DB connection after a failed migration can run with foreign keys disabled, weakening referential integrity and making it easier for bugs or abuse to leave inconsistent rows (e.g. dangling references) that would normally be rejected.

No automated thread IDs were available from this environment to call ResolveReviewThreads; prior automation threads were requested to be cleared via cleanup_previous on this review.

_{Sent by Cursor Automation: Find vulnerabilities}

cursor · 2026-05-04T21:34:05Z

 	}

+	if d.GetDialect() == util.DbDriverSQLite {
+		_, err = d.Sql().Exec("PRAGMA foreign_keys = OFF")


Medium — integrity / defense-in-depth

PRAGMA foreign_keys = OFF is executed before Begin(), but if Begin() fails (return err at ~202) or any of the early return err paths after Rollback() (~213–214, ~247–248, ~253–254) run, the connection never reaches the new PRAGMA foreign_keys = ON block (post-commit). SQLite keeps FK enforcement off for that connection until something else turns it on.

Impact: After a failed migration, subsequent operations on the same pool connection can insert/update rows that violate FK constraints, weakening DB integrity and complicating exploitation of other bugs.

Fix direction: defer a function that sets foreign_keys = ON when SQLite, or use a small helper that always restores on every exit path (including Begin failure and rollbacks).

fiftin added 2 commits May 3, 2026 11:28

refactor(repos): allow nil for key id

a0e5316

feat(migration): nullable key column in repo

2a0dc9d

cursor Bot reviewed May 3, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed May 3, 2026

View reviewed changes

test(unit): fix tests after refactoring

44c37bf

cursor Bot reviewed May 3, 2026

View reviewed changes

Merge branch 'develop' into refactor/nullable_repo_key

616d2a6

cursor Bot reviewed May 4, 2026

View reviewed changes

fix(repos): nil key correct handling

7ee53d2

cursor Bot reviewed May 4, 2026

View reviewed changes

		@@ -42,12 +42,14 @@ func (c GoGitClient) getAuthMethod(r GitRepository) (transport.AuthMethod, error
		switch r.Repository.SSHKey.Type {

Uh oh!

Conversation

fiftin commented May 3, 2026

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Security review (PR 3826)

Uh oh!

cursor Bot May 3, 2026

Choose a reason for hiding this comment

Uh oh!

cursor Bot May 3, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 3, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot May 3, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot May 3, 2026

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Security review (PR #3826)

Uh oh!

cursor Bot May 3, 2026

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Security review (PR #3826)

Uh oh!

cursor Bot May 4, 2026

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Security review (PR 3826)

Uh oh!

cursor Bot May 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cursor Bot left a comment •

edited

Loading

cursor Bot left a comment •

edited

Loading

cursor Bot left a comment •

edited

Loading